-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert "refactor: remove serialize-javascript (#3278)" #3304
Conversation
This reverts commit 13c1f5f.
🦋 Changeset detectedLatest commit: 38f8fb7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Before we merge... do we want to support passing a custom class as a prop for the browser? I can't imagine how we could ever support that technically. Maybe we just call this intentionally not supported, and ask the user to serialize to a JS/JSON object instead. I don't love that error message though, so if we went that route I'd turn this PR into something that creates a more clear error message with some helpful info on how to resolve. |
@FredKSchott We need to define what we support, currently it's been "whatever serialize-javascript supports" and thus this regression. Rather than define it on the fly I think we need to take a step back and reassess the tradeoffs (The PR that removed individual script tags was reverted because of this issue) and make a decision on what to support, and then move forward with implementations. |
Thoughts on using I'm not against reverting if we need to, but this feels small enough that we could that "what is expected behavior?" convo here now (or in Discord) vs. reverting and then having the convo. This doesn't feel like something impacting many users, based on the bug description. |
My concern is that we might want to break this again in the future to fix the "one script tag per island" problem and I'd rather come to one conclusion on what we support and break once than multiple times. |
ah, gotcha. Okay, that makes sense to me. LGTM with either path. |
…astro#3304) * Revert "refactor: remove serialize-javascript (withastro#3278)" This reverts commit 13c1f5f. * Adds a changeset
* Removed ignores * Migration to v3 * More changes * Remove legacy redirects * Fail when there is no ENABLE_VC_BUILD * Fix edge * Updated readme * Changeset * Added static mode * Updated documentation * Updated shim * Made edge work! * Updated changeset * Ensure empty dir * Fixed redirects for dynamic paths * Removed extra declaration * Splited imports * Updated readme * Fixed some urls * Deprecated shim! * [test]: Vercel NFT * Beautify * Edge bundle to node 14.19 Vercel runs 14.19.1 (I've checked it manually) * Re-added shim (#3304) * Added `node:` prefix * Use the same bundling as Deno for Edge * Remove esbuild * Fixed shim * Moved nft * Updated changeset * Added note about Edge * fix typo * Added support for Node 16 (vercel/vercel#7772)
…astro#3304) * Revert "refactor: remove serialize-javascript (withastro#3278)" This reverts commit 13c1f5f. * Adds a changeset
This reverts commit 13c1f5f.
Changes
devalue
caused some regressions. Reverting to fix this.Testing
No
Docs
N/A, bug fix